fix: Disable Hardware Authenticator identities for unsupported Neighborhoods #75

stanleyjones · 2023-09-14T18:25:47Z

Supported Neighborhood

Unsupported Neighborhood

Switching to an Unsupported Neighborhood

Closes #68
Closes #42
Closes #69

…orhoods

netlify · 2023-09-14T18:26:11Z

✅ Deploy Preview for lifted-gwen ready!

Name	Link
🔨 Latest commit	`cab8c4a`
🔍 Latest deploy log	https://app.netlify.com/sites/lifted-gwen/deploys/651ee9802d8b0e0008359aba
😎 Deploy Preview	https://deploy-preview-75--lifted-gwen.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/api/neighborhoods/provider.tsx

src/features/accounts/components/accounts-menu/accounts-menu.tsx

src/features/accounts/components/accounts-menu/add-account-modal.tsx

stanleyjones · 2023-09-14T18:30:16Z

src/features/accounts/components/accounts-menu/add-account-modal.tsx

- );
- })}
+ {createCards
+ .filter((c) => !c.requires || services.has(c.requires))


Here's the visibility logic: Either don't have a requirement or have a satisfied requirement.

stanleyjones · 2023-09-14T18:30:25Z

src/features/accounts/components/accounts-menu/add-account-modal.tsx

- );
- })}
+ {importCards
+ .filter((c) => !c.requires || services.has(c.requires))


stanleyjones · 2023-09-14T18:32:15Z

If this solution is acceptable I'll apply it to liftedinit/alberto#135 too.

stanleyjones · 2023-09-14T18:33:08Z

Investigating CI failure.

codecov · 2023-09-14T20:18:59Z

Codecov Report

Merging #75 (cab8c4a) into main (26510c9) will increase coverage by 1.01%.
The diff coverage is 53.42%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   29.15%   30.16%   +1.01%     
==========================================
  Files          61       61              
  Lines        1012     1041      +29     
  Branches      208      212       +4     
==========================================
+ Hits          295      314      +19     
- Misses        714      724      +10     
  Partials        3        3

Files	Coverage Δ
src/features/accounts/api/create-account.ts	`0.00% <0.00%> (ø)`
src/features/accounts/api/get-account-info.ts	`0.00% <0.00%> (ø)`
...nts/components/accounts-menu/add-account-modal.tsx	`98.27% <93.33%> (+0.12%)`	⬆️
src/pages/services/blocks/blocks.tsx	`0.00% <0.00%> (ø)`
...pages/services/compute/close-deployment-dialog.tsx	`0.00% <0.00%> (ø)`
src/pages/services/compute/compute.tsx	`0.00% <0.00%> (ø)`
...pages/services/compute/create-deployment-modal.tsx	`4.25% <0.00%> (ø)`
src/pages/services/data/delete-key-dialog.tsx	`0.00% <0.00%> (ø)`
src/pages/services/data/mark-immutable-dialog.tsx	`0.00% <0.00%> (ø)`
src/pages/services/data/put-value-modal.tsx	`0.00% <0.00%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jgryffindor · 2023-09-15T03:20:24Z

I tested this on https://dev002_gwen.liftedinit.tech/ and functionally looks good!

fmorency

I like it, thanks @stanleyjones

@jgryffindor can we deploy this on QA? I want to test using an HSM.

fmorency

I tested using a Yubikey on https://qa-gwen.liftedinit.tech/.

The identity switch to Anonymous selecting an unsupported network doesn't work. Instead, it asks me to touch the key. The new network is selected, but the identity stays the same.

jgryffindor · 2023-09-15T15:31:50Z

When trying to import my existing ledger wallet I get an error "You're using a security key that's not registered..." and when creating a new account I get an error for Anonymous as well.

fmorency · 2023-09-15T15:34:59Z

@jgryffindor the first error is expected. The other is not. Did you select the QA Ledger network when creating the new account?

jgryffindor · 2023-09-15T15:39:16Z

Woops, yah disregard the 2nd.

Creating account flow with Ledger works as expected for the QA ledger.

After login, I switched to the QA KVstore network and it asked me to use the key to log in. It allowed me to select QA KVstore and appeared to be logged in with HWW. I went to Services > Data and it prompted me to login again. It then loops asking for login, so I think we need to catch that sequence and prevent switching to KVStore after logging into Ledger networks with Ledger HWW.

stanleyjones · 2023-09-21T17:09:32Z

@fmorency @jgryffindor How are you able to create an HSM identity on qa-gwen.liftedinit.tech? I receive the following error when I try:

Could not verify the signature: Unknown error: Anonymous requires no key id.,
Could not verify the signature: signature error., Unknown error: Origin not allowed.

Update: Ah... this is what @jgryffindor is talking about above... It works when I switch to "QA Ledger." Now I'm getting the same error: prompted on the HSM but not switching to Anonymous.

… along with available services

stanleyjones

Made some changes to the NeighborhoodContext to address the current problems.

NeighborhoodContext now provides both a query and a command network.
The set of services is updated when the neighborhood changes, not by the component.

stanleyjones · 2023-09-21T20:55:39Z

src/api/neighborhoods/provider.tsx

+interface INeighborhoodContext {
+ query?: Network;
+ command?: Network;
+ services: Set<string>;
+}


Instead of returning just a Network (using the active Identity), the context is now a little more complex with a Network for queries (anonymous) and one for commands (using the active Identity). Also, the services are computed once and stored in the context, instead of queried at the component level.

stanleyjones · 2023-09-21T20:56:19Z

src/api/neighborhoods/provider.tsx

+ const query = new Network(url, anonymous);
+ const command = new Network(url, identity);


Creating the two Networks here...

stanleyjones · 2023-09-21T20:56:53Z

src/api/neighborhoods/provider.tsx

+ return;
+ }
+ const { endpoints } = await context.query.base.endpoints();
+ context.services = endpoints


...and updating the services, only when the networks have changed.

stanleyjones · 2023-09-21T20:58:00Z

src/api/neighborhoods/provider.tsx

 {children}
 </NeighborhoodContext.Provider>
 );
 }
+
+export const useNeighborhoodContext = () => useContext(NeighborhoodContext);


Finally, return a useNeighborhoodContext custom hook to reduce imports throughout the codebase.

stanleyjones · 2023-09-21T20:59:04Z

src/features/accounts/api/create-account.ts

@@ -20,7 +19,7 @@ export type CreateAccountFormData = {
 };

 export function useCreateAccount() {
- const n = useContext(NeighborhoodContext);
+ const { command: n } = useNeighborhoodContext();


The change to context required a lot of changes. I approached them by changing as little code as possible, mostly destructuring using the existing variable name so I didn't have to touch the rest of the code. I had to decide whether the function needed the query or command network — mutations got command, queries got query.

stanleyjones · 2023-09-21T21:01:03Z

src/features/accounts/components/accounts-menu/accounts-menu.tsx

+ (async () => {
+ const isWebAuthnIdentity =
+ activeAccount?.identity instanceof WebAuthnIdentity;
+ if (isWebAuthnIdentity && !services.has("idstore")) {


Getting the list of services is no longer asynchronous, which I suspect was causing a problem here.

stanleyjones · 2023-09-21T21:02:16Z

src/features/accounts/components/accounts-menu/add-account-modal.tsx

@@ -172,18 +173,21 @@ function CreateAccountOptions({
 }: {
 onAddMethodClick: (method: AddAccountMethodTypes) => void;
 }) {
+ const { services } = useNeighborhoodContext();


Now that services is part of the context, I just pull it out at the component level instead of setting it in the container and passing it down through props.

fmorency · 2023-09-22T14:19:45Z

Thanks @stanleyjones.

Is there a reason why you diverged from how Alberto is doing this? We already had support for query/command/legacy Network in the NetworkContext. I think I prefer your version but still, we should reconcile things at some point.

@jgryffindor can we deploy this to QA for testing?

jgryffindor · 2023-09-25T13:42:10Z

Test is deployed to QA @stanleyjones

fmorency · 2023-09-25T14:19:32Z

I tested the feature in QA and it doesn't appear to work. I imported an existing account from the QA Ledger. The account was imported but Gwen reverted back to Anonymous instantly. Selecting the HSM account from the account menu always reverts back to Anonymous, even if the network has HSM support.

stanleyjones · 2023-09-25T16:14:56Z

Is there a reason why you diverged from how Alberto is doing this? We already had support for query/command/legacy Network in the NetworkContext. I think I prefer your version but still, we should reconcile things at some point.

Agreed that they need to be reconciled. My thought process was this:

Hmm... Alberto has something different but I don't know what "legacy networks" are. Let's make sure they can be added in the future.
Even though JavaScript will let you, I don't like the idea of an array where the first two elements are of the same type but optional and the third and fourth elements are arrays of two different types.
I'll make it an object so everything has a clear name and people don't have to remember/guess which position is which thing. Then if we need to add legacy networks to Gwen, we won't have to change the order of everything.

Otherwise I think the context would be [Network?, Network?, Network[], Set<string>] which is a little cringe.

stanleyjones · 2023-09-25T16:15:37Z

I tested the feature in QA and it doesn't appear to work.

Okay, I'll investigate. Thanks. Frustrating that we have to deploy to QA for these issues to arise.

fmorency · 2023-09-26T14:18:59Z

Hmm... Alberto has something different but I don't know what "legacy networks" are. Let's make sure they can be added in the future.

Yeah :(. This is a workaround for the chain breakage we had a while ago. Time was of the essence and I never revisited this (bad) design.

I don't like the idea of an array where the first two elements are of the same type but optional and the third and fourth elements are arrays of two different types.

Yes, I agree.

Otherwise I think the context would be [Network?, Network?, Network[], Set<string>] which is a little cringe.

Thanks for reworking that shady part of the code. I will port your design to Alberto and take this opportunity to rework the workaround mess.

stanleyjones · 2023-09-28T18:13:00Z

I believe I found the bug. Services were being correctly fetched but not set in the context object used by the account menu component. I still can't fully test this locally since I can't import an HSM for localhost but I've confirmed that the check for idstore passes when connected to a neighborhood with that module (i.e. the second half of this conditional, which previously was always true if (isWebAuthnIdentity && !services.has("idstore"))).

fmorency · 2023-09-29T17:24:22Z

@jgryffindor can we deploy this on QA for testing please?

jgryffindor · 2023-10-03T17:01:34Z

QA Gwen is updated. Definitely looks better! When switching to a kvstore network with HSM selected I get alerted the network doesn't support it. https://qa-gwen.liftedinit.tech

fmorency

The issue appears to be fixed, thanks.

nit: selecting QA Ledger and then selecting Demo Compute or Demo KvStore keeps the HSM identity. The other networks behave properly.

@stanleyjones I'll let you decide if you want to merge as-is or fix it in this PR. Anyhow, you can merge.

stanleyjones · 2023-10-05T16:50:42Z

It looks like it's getting a CORS error when trying to connect to those networks, so it keeps the list of available services from the previous network. I made it reset each time and handle the error.

stanleyjones · 2023-10-05T16:53:34Z

Waiting for CI and then I'll merge.

fix: Disable Hardware Authenticator identities for unsupported Neighb…

8548843

…orhoods

stanleyjones self-assigned this Sep 14, 2023

stanleyjones requested review from fmorency and jgryffindor September 14, 2023 18:25

stanleyjones commented Sep 14, 2023

View reviewed changes

fix: Handle undefined neighborhood

a4530dc

stanleyjones mentioned this pull request Sep 14, 2023

feat: Disable Hardware Authenticator identities for unsupported Networks liftedinit/alberto#140

Merged

fmorency reviewed Sep 15, 2023

View reviewed changes

fmorency requested changes Sep 15, 2023

View reviewed changes

stanleyjones added 2 commits September 21, 2023 13:44

fix: Modify NeighborhoodContext to include query and command networks…

9d1c8e8

… along with available services

fix: Memoization dependencies

aa4d936

stanleyjones commented Sep 21, 2023

View reviewed changes

fmorency mentioned this pull request Sep 27, 2023

feat: refactor network provider liftedinit/alberto#144

Merged

fix: Update services in context

ff7747a

stanleyjones requested a review from fmorency September 28, 2023 18:15

fmorency approved these changes Oct 4, 2023

View reviewed changes

fix: Handle error retrieving endpoints

cab8c4a

stanleyjones merged commit 940e446 into liftedinit:main Oct 5, 2023
10 checks passed

stanleyjones deleted the disable-hardware-authenticators branch October 5, 2023 16:57

stanleyjones mentioned this pull request Oct 5, 2023

Disable Hardware Authenticator entry if neighborhood doesn't support it liftedinit/alberto#135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Disable Hardware Authenticator identities for unsupported Neighborhoods #75

fix: Disable Hardware Authenticator identities for unsupported Neighborhoods #75

stanleyjones commented Sep 14, 2023 •

edited

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

stanleyjones Sep 14, 2023

stanleyjones Sep 14, 2023

stanleyjones commented Sep 14, 2023

stanleyjones commented Sep 14, 2023

codecov bot commented Sep 14, 2023 •

edited

Loading

jgryffindor commented Sep 15, 2023

fmorency left a comment

fmorency left a comment

jgryffindor commented Sep 15, 2023 •

edited

Loading

fmorency commented Sep 15, 2023

jgryffindor commented Sep 15, 2023 •

edited

Loading

stanleyjones commented Sep 21, 2023 •

edited

Loading

stanleyjones left a comment

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

stanleyjones Sep 21, 2023

fmorency commented Sep 22, 2023

jgryffindor commented Sep 25, 2023

fmorency commented Sep 25, 2023

stanleyjones commented Sep 25, 2023

stanleyjones commented Sep 25, 2023

fmorency commented Sep 26, 2023

stanleyjones commented Sep 28, 2023

fmorency commented Sep 29, 2023

jgryffindor commented Oct 3, 2023

fmorency left a comment •

edited

Loading

stanleyjones commented Oct 5, 2023

stanleyjones commented Oct 5, 2023

		const query = new Network(url, anonymous);
		const command = new Network(url, identity);

fix: Disable Hardware Authenticator identities for unsupported Neighborhoods #75

fix: Disable Hardware Authenticator identities for unsupported Neighborhoods #75

Conversation

stanleyjones commented Sep 14, 2023 • edited Loading

Supported Neighborhood

Unsupported Neighborhood

Switching to an Unsupported Neighborhood

netlify bot commented Sep 14, 2023 • edited Loading

✅ Deploy Preview for lifted-gwen ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanleyjones commented Sep 14, 2023

stanleyjones commented Sep 14, 2023

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

jgryffindor commented Sep 15, 2023

fmorency left a comment

Choose a reason for hiding this comment

fmorency left a comment

Choose a reason for hiding this comment

jgryffindor commented Sep 15, 2023 • edited Loading

fmorency commented Sep 15, 2023

jgryffindor commented Sep 15, 2023 • edited Loading

stanleyjones commented Sep 21, 2023 • edited Loading

stanleyjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmorency commented Sep 22, 2023

jgryffindor commented Sep 25, 2023

fmorency commented Sep 25, 2023

stanleyjones commented Sep 25, 2023

stanleyjones commented Sep 25, 2023

fmorency commented Sep 26, 2023

stanleyjones commented Sep 28, 2023

fmorency commented Sep 29, 2023

jgryffindor commented Oct 3, 2023

fmorency left a comment • edited Loading

Choose a reason for hiding this comment

stanleyjones commented Oct 5, 2023

stanleyjones commented Oct 5, 2023

stanleyjones commented Sep 14, 2023 •

edited

Loading

netlify bot commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

jgryffindor commented Sep 15, 2023 •

edited

Loading

jgryffindor commented Sep 15, 2023 •

edited

Loading

stanleyjones commented Sep 21, 2023 •

edited

Loading

fmorency left a comment •

edited

Loading